Add front-end linter and formatter#11
Conversation
jonhoo
left a comment
There was a problem hiding this comment.
Thanks for taking this on! It generally looks good, just some small minor notes (questions really).
| } | ||
| // clear next batch to go | ||
| resolve(true); | ||
| resolve && resolve(true); |
There was a problem hiding this comment.
Huh, why is the resolve && bit needed here?
There was a problem hiding this comment.
Eslint can't tell that resolve will always be assigned (since it is assigned inside the Promise(() => {}) function) so it complains that resolve might be undefined here.
The && is just an old habit I picked up from previous JS codebases I worked on. This code is roughly equivalent to:
// `!!` coerces resolve into a boolean, which is what the && is doing implicitly above
// !!undefined => 0
// !!(() => {}) => true -- this works with any function
if (!!resolve) { resolve(true) }Since we are using ecma2020 features, I could also change it to resolve?.(true) by using the optional chaining operator.
I'm don't think there is an equivalent to the ! typescript operator (not null/undefined) in javascript, so if we want to make the linter happy without disabling it we'll need some of check (&&/if) or optional chaining.
If we want the code to be more explicit and not rely on quirks like coercion or newer features like ?, then I would probably go with if (resolve !== undefined) { resolve(true) }
There was a problem hiding this comment.
I went ahead and changed it to the explicit if (resolve !== undefined) { resolve(true) } check in 06a283d. Let me know if you would prefer the optional chaining option, or something else entirely.
There was a problem hiding this comment.
My thinking here was more that if this is ever undefined, I'd want us to error, rather than just silently not call resolve the way we would given a construct like this. Tracking down failures to resolve are super painful :p
There was a problem hiding this comment.
I was sure I replied to this over the weekend, but I must have forgotten to click on comment and lost the previous message 😢.
You raised a good point, I was initially just trying to bypass this limitation of the type system (since the callback to promises is synchronous so it's not possible for resolve to ever be undefined with the current code. But I hadn't considered that future code changes could lead to frustrating troubleshooting.
The executor is called synchronously (as soon as the Promise is constructed) with the resolveFunc and rejectFunc functions as arguments. (https://developer.mozilla.org/enUS/docs/Web/JavaScript/Reference/Global_Objects/Promise/Promise)
I'll revert the code back to what it was before.
More Robust Fix Suggestion
For a more permanent fix though, the following idea came to mind:/**
* SAFETY: Since the Promise constructor (executor) is ran synchronously on Promise creation
* both `resolve` and `reject` will not be `undefined` when returned from this function.
* https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Promise/Promise
* @returns {Promise & { resolve: (value: unkown) => void, reject: (reason?: any) => void}
*/
function deferrablePromise() {
let resolve;
let reject;
const promise = new Promise((rs, rj) => {
resolve = thisResolve;
reject = thisReject;
});
return Object.assign(promise, { resolve, reject });
}
/* ... */
fetch_done = deferrablePromise(); // Create a promise that holds it's own resolve function
/* ... */
fetch_done.resolve(true)The deferrablePromise would isolate the undefined issue to a single code point, and this deferrablePromise could also be used for batch, fetching and a few other places to simplify the code a little more.
Put 1.70 in there (for instance if you want to pin against OnceLock stabilizing) and it will actually test 1.7 as it appears github auto converts this to a float? Putting in quotes seems to do the right thing here
Step 1 of #10.
Lint and Format
npm commands:
npm run lint&&npm run formatI used the configuration from
npm create svelte@latestas a reference for Svelte linting and formatting best practices, since I am not familiar with it. To help organize Tailwind classes and catch conflicting classes, I added the prettier-plugin-tailwindcss package. However, I had to disable auto-loading of the prettier plugin due to a limitation. Also, I had to changetailwind.config.cjsto a CommonJS module, because it was not being loaded properly otherwise.I fixed a few small linting and formatting errors and warnings to pass the linter. However, it might be difficult to track these changes with all the other formatting changes introduced in this PR. Here is a quick list of the fixes:
respvariable inQuestion.svelterejectvariable instore.jsreject1,resolve, andrejectin a few anonymous functions instore.js, since they were not being usedborderproperty in a few places in the Svelte components, as theborder-2property was overridingCI
I added a Github Actions that runs the linter and formatter for any PR into the
mainbranch. Up to you if you want to add a branch protection rule for this action.I considered adding Husky to run a lint and format pre-commit hook locally, but decided against it to keep things simple. If someone wants this functionality, they can create a local git-hook.
Notes
The npm commands were copied as is from the
npm create svelte@latestapp and target any files that they know how to format, including JSON files, configuration files, and README code blocks. I left them as is, since I don't see any harm in it. If you want to reduce the number of changes in this PR, I can restore those files and set the scripts to only target js and svelte files in our source directory.